-
Notifications
You must be signed in to change notification settings - Fork 4
fix: prevent synced invoices from getting updated by outdated webhooks #68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fe7dce9
to
04d51a1
Compare
.map((x) => `"${x}" = EXCLUDED."${x}"`) | ||
.join(',')}, | ||
last_synced_at = :last_synced_at | ||
WHERE "${table}"."last_synced_at" IS NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will upsert if ANY entry in the table matches this, but not necessarily this id, we can do something like
WHERE NOT EXISTS (select 1 from "${table}" where id = :id AND "${table}"."last_synced_at" > :last_synced_at;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The integration tests likely did not catch this (if true), given you test for a single entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! You're right - I'll add a test to cover this too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to do a deep dive into the Postgres docs, but it seems that Postgres actually scopes the WHERE
clause in ON CONFLICT DO UPDATE
very narrowly.
Specifically:
- The WHERE clause only runs after a conflict is detected (i.e. once a row with matching constraint is found).
- At that point, it only has access to two rows:
- The conflicting row from the table (via
${table}
or alias) - The proposed insert row (via
excluded
)
- The conflicting row from the table (via
So even though it looks like ${table}."last_synced_at"
could match any row, it actually only refers to the row that caused the conflict.
Source:
- PostgreSQL Docs on INSERT
“The SET and WHERE clauses in ON CONFLICT DO UPDATE have access to the existing row using the table’s name (or an alias), and to the row proposed for insertion using the special excluded table.” - This comment on StackOverflow also says the same thing and has a clearer example: https://stackoverflow.com/a/44579804
That said, I find this to be quite non-intuitive (even a bit misleading 😅). I've added a test to verify that this works and reviewed it thoroughly, and I believe it addresses this case. I've added a comment too.
Let me know what you think :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sgtm, also tested this locally and it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
3860995
to
21163f2
Compare
🎉 This PR is included in version 1.11.14 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR introduces a fix for out-of-order webhooks causing erroneous states in the invoices.
What is the current behavior?
Out-of-order webhook events from Orb can cause incorrect invoice states in the database. Since the system upserts invoices without checking the recency of the state, an issued status may overwrite a previously stored paid status.
What is the new behavior?
This PR introduces a general safeguard for upserting - the
upsertManyWithTimestampProtection
function. This function uses a SQL statement with aWHERE
clause that only updates records when the incominglast_synced_at
timestamp is newer than the existing one. This prevents older webhook events from overwriting newer data, ensuring data consistency. This pattern can easily be applied to other entities by using the same function in their respective sync functions.It also:
invoices
,customers
,subscriptions
,credit_notes
,plans
, andbillable_metrics
syncTimestamp
parameter to thesyncInvoices
method allows webhook handlers to pass the webhook'screated_at
timestamp, which is then used as thelast_synced_at
value for timestamp-based protection